-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Notebook server info files #4772
Conversation
self.remove_server_info_file() | ||
|
||
|
||
def discover_running_servers(profile='default'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like this should be defined somewhere other than the notebookapp file, but I'm not sure where. Any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a brief look, but this seemed the most logical place I could see. I'd argue that it's effectively 'list instances of NotebookApp on this system', so it makes sense to define it near the NotebookApp class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to add an ipython notebook list
subcommand to show currently running servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am +1 on having a notebook list
subcommand, but I think we should name the function using "list" instead of "discover": discover_running_servers
.
In case the server fails to start, could the file contain the corresponding traceback error? That would be useful to show possible errors to users in a GUI dialog when the server is started in the background. |
The idea is that the file only exists when the server is running, so if it fails to start, the file shouldn't be present. Perhaps there should be an option which makes the server emit more structured (e.g. JSON) messages on stdout, for a controlling process to monitor. Another example would be for synchronisation - you don't want to try to read the server info file until the server has finished writing it. |
'secure': bool(self.certfile), | ||
'baseurlpath': self.base_project_url, | ||
'notebookdir': os.path.abspath(self.notebook_manager.notebook_dir), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably keep the same names in JSON, so base_project_url
and notebook_dir
.
Overall, I think this looks good, just some minor changes to make. |
|
if self.json: | ||
print(json.dumps(serverinfo)) | ||
else: | ||
print(serverinfo['url'], "::", serverinfo['notebook_dir']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to add [...]
so that the full JSON output is valid JSON, not just each entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in dev meeting. Conclusion: it's more trouble than it's worth to get commas in the right places, so I'm leaving it as one JSON object per line for now.
Notebook server info files
This adds info files for running notebook servers, akin to the connection files we already write for kernels, along with a function to discover them. This should facilitate making tools that can open notebooks without starting a new server for each one.
What values are serialised and what names they're given are up for discussion. I've currently put in the URL and its component parts and the root directory where the server is running.